-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Material Support to Globe #5919
Conversation
…now uses a color ramp image
@jasonbeverage this is super exciting, thanks again! Here's Seneca Rocks where I use to rock climb: For the contours, consider adding an optional where they "maintain" constant pixel width using screen-space partial derivatives. Perhaps something like this: https://github.com/AnalyticalGraphicsInc/cesium/blob/a81f2e7de64c2348558046b5e8e685278fde0dcf/Source/Shaders/Materials/GridMaterial.glsl#L23 @lilleyse can you take a first pass at this? I would like to do a quick review before merging. |
That's awesome you used to rock climb in Seneca Rocks! I'm from West
Virginia so I've been there a bunch as a kid and I went camping and trout
fishing in that area last year with my uncle.
…On Sat, Oct 21, 2017 at 3:33 PM Patrick Cozzi ***@***.***> wrote:
@jasonbeverage <https://github.com/jasonbeverage> this is super exciting,
thanks again!
Here's Seneca Rocks where I use to rock climb:
[image: image]
<https://user-images.githubusercontent.com/782098/31855226-f10226ae-b674-11e7-8435-a9bb865763ad.png>
For the contours, consider adding an optional where they "maintain"
constant pixel width using screen-space partial derivatives. Perhaps
something like this:
https://github.com/AnalyticalGraphicsInc/cesium/blob/a81f2e7de64c2348558046b5e8e685278fde0dcf/Source/Shaders/Materials/GridMaterial.glsl#L23
@lilleyse <https://github.com/lilleyse> can you take a first pass at
this? I would like to do a quick review before merging.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5919 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAT74nlgO05xAWOOYOt_uojSFergVoTnks5sukccgaJpZM4QBF3x>
.
|
Hi, I also did this job but I just modified the globefs directly, I never thought adding material to support globe, it is a decent design, I gained it. Here is my little suggestion to set the contour line. float dContourInterval = 100.0;// the interval value of contour line material.diffuse = color.rgb; return material; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great so far! I have a few structural comments, but overall the approach is solid.
}); | ||
viewer.terrainProvider = cesiumTerrainProviderMeshes; | ||
|
||
viewer.scene.globe.enableLighting = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The demo works better without lighting. Related to my comment below, we should be able to get the normals properly without this being set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color ramps are better with lighting so you can see the terrain features. The terrain all blends together without the shadows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lighting does look fine most of the time, it's mainly when it's completely dark that it's hard to see what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. For example, there are some random Appalachian mountains with and without lighting:
Having the lighting enabled makes a big difference.
To make sure the lighting always looks good with the preset views, we can add a line to set the clock time:
viewer.clockViewModel.currentTime = Cesium.JulianDate.fromDate(new Date(2017, 8, 22, 0, 0, 0));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yeah, setting the clock is good. I should have been clearer, my main issue was that whenever I would open the demo it would be night at Everest and just hard to see anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a change to set the clock time.
viewer.scene.globe.material.uniforms.color = Cesium.Color.RED; | ||
} | ||
}, { | ||
text : 'Slope Color Ramp Material', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This material doesn't work with enableLighting
off.
If the material requires normals you can pass in the define GENERATE_POSITION_AND_NORMAL
to the globe vertex shader.
For terrain that doesn't contain normals the material should be ignored so it doesn't appear black.
Source/Scene/Globe.js
Outdated
@@ -525,6 +540,10 @@ define([ | |||
return; | |||
} | |||
|
|||
if (this._material) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use defined(this._material)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Source/Scene/Globe.js
Outdated
|
||
var fragmentSources = []; | ||
var fragmentDefines = []; | ||
if (this._material) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Source/Scene/Globe.js
Outdated
fragmentDefines.push('APPLY_MATERIAL'); | ||
|
||
// Set the material uniform map to the materials | ||
this._surface._tileProvider.uniformMap = this._material._uniforms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uniformMap
should be defined in the constructor of GlobeSurfaceTileProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a better name would be materialUniformMap
.
@@ -1262,6 +1264,10 @@ define([ | |||
uniformMapProperties.minMaxHeight.y = encoding.maximumHeight; | |||
Matrix4.clone(encoding.matrix, uniformMapProperties.scaleAndBias); | |||
|
|||
if (tileProvider.uniformMap) { | |||
uniformMap = combine(uniformMap, tileProvider.uniformMap); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be best to combine uniforms elsewhere since this will allocate a uniform map object every frame.
I wonder if this could be moved to the area near if (tileProvider._drawCommands.length <= tileProvider._usedDrawCommands) {
. Can it detect when the material is dirty and combine up there?
Source/Shaders/GlobeVS.glsl
Outdated
@@ -21,6 +21,8 @@ varying vec3 v_positionEC; | |||
varying vec3 v_textureCoordinates; | |||
varying vec3 v_normalMC; | |||
varying vec3 v_normalEC; | |||
varying float v_slope; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in here should also be surrounded by the APPLY_MATERIAL
define too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Source/Shaders/GlobeFS.glsl
Outdated
@@ -61,6 +61,8 @@ varying vec3 v_positionEC; | |||
varying vec3 v_textureCoordinates; | |||
varying vec3 v_normalMC; | |||
varying vec3 v_normalEC; | |||
varying float v_height; | |||
varying float v_slope; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surround these with the APPLY_MATERIAL
define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -1262,6 +1264,10 @@ define([ | |||
uniformMapProperties.minMaxHeight.y = encoding.maximumHeight; | |||
Matrix4.clone(encoding.matrix, uniformMapProperties.scaleAndBias); | |||
|
|||
if (tileProvider.uniformMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use defined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Added some style tweaks in 60c0121 |
Remaining TODO:
|
Source/Scene/Material.js
Outdated
uniforms : { | ||
image: Material.DefaultImageId, | ||
minHeight: 0.0, | ||
maxHeight: 10000.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change these to minimumHeight
and maximumHeight
. We try to avoid abbreviations so there's no confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the dangling comma here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@jasonbeverage are you able to make the suggested changes to get this over the finish line for the upcoming 1.40 release? 😄 |
Hey Patrick I'll work on getting these addressed real soon. Thanks!
…On Sun, Nov 12, 2017 at 2:47 PM Patrick Cozzi ***@***.***> wrote:
@jasonbeverage <https://github.com/jasonbeverage> are you able to make
the suggested changes to get this over the finish line for the upcoming
1.40 release? 😄
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5919 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAT74r4tBfgThaLw3cZ2fjTJIXJTc3kxks5s10s5gaJpZM4QBF3x>
.
|
Very nice, rock on @jasonbeverage! |
…cent on first view of Mt Everest
Added implementation of screen space partial derivatives for contour lines to make them look nicer. Source/Shaders/Materials/ElevationContourMaterial.glsl
I just pushed a change to the ElevationContourMaterial that uses the code from @pasu to implement the screen space partial derivative based contouring. I also added a new lineThickness uniform so you can control the thickness of the line. It looks pretty good to me. I think my recent changes addresses everything mentioned here. Let me know if you see anything else! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonbeverage this is good stuff as always! We would like to include this in the 1.40 release going out on Friday so we would like to get this into master ASAP.
Do you have time to
(1) make the mostly trivial changes I requested?
(2) add unit tests?
Thanks!!!
CHANGES.md
Outdated
@@ -3,6 +3,7 @@ Change Log | |||
|
|||
### 1.40 - 2017-12-01 | |||
|
|||
* Adds Material support to Globe. [#5919](https://github.com/AnalyticalGraphicsInc/cesium/pull/5919) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this link to the future Sandcastle example that will live here: https://cesiumjs.org/Cesium/Apps/Sandcastle/
ramp.height = 1; | ||
var ctx = ramp.getContext('2d'); | ||
|
||
var values = selectedShading === 'elevation' ? elevationRamp : slopeRamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hpinkos can the color ramp be tweaked to be a bit less subtle for the default view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or change the default view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the default view to be of the Himalayas
Source/Scene/Globe.js
Outdated
/** | ||
* @private | ||
*/ | ||
Globe.prototype.dirtyShaders = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would generally name this a verb like makeShadersDirty
and not make it on the Globe
object; instead, it would be a stand-alone function at file level scope and then we pass in a Globe
object. This provides better encapsulation.
Source/Scene/Globe.js
Outdated
fragmentSources.push(this._material.shaderSource); | ||
defines.push('APPLY_MATERIAL'); | ||
|
||
// Set the material uniform map to the materials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't need this comment.
Source/Scene/Globe.js
Outdated
|
||
this._surfaceShaderSet.baseVertexShaderSource = new ShaderSource({ | ||
sources : [GroundAtmosphere, GlobeVS], | ||
defines: defines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout, we would generally write defines :
with a space before the semicolon. The auto formatter in your IDE probably does this. WebStorm does.
Source/Scene/Material.js
Outdated
* <ul> | ||
* <li><code>color</code>: color and alpha for the contour line.</li> | ||
* <li><code>spacing</code>: spacing for contour lines in meters.</li> | ||
* <li><code>lineThickness</code>: Number specifying the thickness of the grid lines in pixels.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the rest of the Cesium API, can this be "width" instead of "thickness?"
@@ -10,6 +10,8 @@ | |||
* @property {vec3} normalEC Unperturbed surface normal in eye coordinates. | |||
* @property {mat3} tangentToEyeMatrix Matrix for converting a tangent space normal to eye space. | |||
* @property {vec3} positionToEyeEC Vector from the fragment to the eye in eye coordinates. The magnitude is the distance in meters from the fragment to the eye. | |||
* @property {float} height The height of the terrain. Only available for globe materials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be more explicit? I assume the height in meters above or below the WGS84 ellipsoid?
@@ -10,6 +10,8 @@ | |||
* @property {vec3} normalEC Unperturbed surface normal in eye coordinates. | |||
* @property {mat3} tangentToEyeMatrix Matrix for converting a tangent space normal to eye space. | |||
* @property {vec3} positionToEyeEC Vector from the fragment to the eye in eye coordinates. The magnitude is the distance in meters from the fragment to the eye. | |||
* @property {float} height The height of the terrain. Only available for globe materials. | |||
* @property {float} slope The slope of the terrain normalized from 0 to 1. Only available for globe materials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be more explicit, e.g., 0 is flat and 1 is vertical or vice versa?
Source/Shaders/GlobeVS.glsl
Outdated
@@ -167,4 +172,11 @@ void main() | |||
v_rayleighColor = atmosColor.rayleigh; | |||
v_distance = length((czm_modelView3D * vec4(position3DWC, 1.0)).xyz); | |||
#endif | |||
|
|||
#ifdef APPLY_MATERIAL | |||
vec3 finalNormal = normalize(v_normalMC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to normalize, it is already done: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Shaders/Builtin/Functions/octDecode.glsl#L24
Source/Shaders/GlobeVS.glsl
Outdated
|
||
#ifdef APPLY_MATERIAL | ||
vec3 finalNormal = normalize(v_normalMC); | ||
vec3 worldNormal = normalize(v_positionMC.xyz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and the line above, we generally try to treat varyings in a vertex shader as "write once, don't read" - more for style - I don't think it matters for performance.
Here, the return from czm_octDecode
could be assigned to a local, and position3DWC
can be used instead of v_positionMC
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worldNormal
is a confusing name - since it is not the terrain normal in world coordinate - perhaps rename to ellipsoidNormal
since this is normal to the ellipsoid?
Hey @pjcozzi I'll try to make those changes tonight or tomorrow. Do you have an example of something I could use for inspiration for the unit tests? I'm still not the best at writing them :) I'm heading out of town for the IITSEC conference on Sunday and won't be back until the end of the week, so I'll try to get what I can wrapped up before I head out. |
@jasonbeverage awesome - it is deeply appreciated! I'm not sure that it is the best example, but here is a simple example for rendering a globe in a unit test: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Specs/Scene/SceneSpec.js#L488-L505 For your unit tests, just cover rendering with each type of material and all the edge cases, e.g., changing material, going from no material to material, from material to no material, perhaps changing uniforms, etc. The test guiding is pretty is good: Also:
|
…ction intead of a private function
I think I've addressed all of your comments except for the unit tests. |
I just added a few unit tests that appear to pass :) I'm heading out for the rest of the day and will be gone until next Wednesday but I'll be answering email and checking this PR if you need anything ;) |
Very nice, thanks again @jasonbeverage. Have a great conference! |
Thanks for merging this! I'll hopefully have at least one more little PR before the end of the year :) |
|
@jasonbeverage. Hello i am trying to implement Cesium globe materials slope, elevation, aspect features in my local project but i am facing issue in my project if i apply any these options my globe is not showing perfect output its look like only single color on my whole globe 🌐 like red or blue or green Thanks |
@Abhishek950650 I would ask your question in the Cesium community forum (https://community.cesium.com/) so others can take a look and help out. Include more information like a Sandcastle and how you're using these materials (see https://community.cesium.com/t/how-to-share-custom-sandcastle-examples/9828) |
Hi all!
This PR adds Material support to the Globe, as well as adding new height and slope values to the czm_materialInput struct so that it can be used as input when generating the materials.
I've added a new sandcastle example that you can play with here
The main goal of this effort was to visualize changes in slope on the terrain, but I tried to make it more generic by utilizing the existing Material support so we could do some more interesting effects in the future. I've added some new materials that you can see in use in the sandcastle example
Some notes on the implementation:
Thanks!
Jason